Merge IAM policy hierarchy by david-crespo · Pull Request #1113 · oxidecomputer/console

david-crespo · 2022-08-11T20:03:39Z

This doesn't feel good exactly but it conveys the information that's currently available and will let us get a feel for the strengths and weakness of the approach so far. I will add some notes on the experience once it's completely ready.

What's in here

Update to API from /policy for current silo, /global/policy for fleet omicron#1573 for /policy endpoint to get current silo policy
Org page displays merged silo and org policies
Project page displays merged silo, org, and project policies
Show all roles for each user
Only show edit or delete on rows for the immediate resource
Highlight "effective" role in green

Followup work

Copy explaining the "effective" role highlighting
Instead of excluding edit/delete from menu, maybe include disabled with explanatory tooltip
Make sure we can handle silo groups too
"Add user to org/project" language on button is very misleading because a user can be added even if they already have access through the parent org or silo. It's more like "give user a role specifically on this project".
The matrix requires referring to the silo role, but we probably shouldn't say "silo"!

vercel · 2022-08-11T20:03:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
console-ui-storybook	✅ Ready (Inspect)	Visit Preview	Aug 16, 2022 at 3:01PM (UTC)

david-crespo · 2022-08-12T22:31:11Z

+  return (
+    <Badge color={effectiveRole === cellRole ? 'default' : 'neutral'}>{cellRole}</Badge>
+  )
+}


This is all kind of ridiculous, but it lets us do the latter instead of something like the the former:

-cell: (info) => <RoleBadge effectiveRole={info.row.original.effectiveRole} role={info.getValue()} /> +cell: RoleBadge

david-crespo · 2022-08-12T23:26:51Z

+        (row) => row.id
+      ),
+    [siloRows, orgRows]
+  )


this logic is almost identical between org and project — however the shared row processing logic I had before was kind of terrible, so I can't bring myself to extract this.

david-crespo · 2022-08-12T23:28:06Z

-        cell: (info) => <Badge color="neutral">{info.getValue()}</Badge>,
+      colHelper.accessor('siloRole', {
+        header: 'Silo role',
+        cell: RoleBadgeCell,


in addition to being very clean, this actually enforces that the row type has effectiveRole on it, which is pretty cool

david-crespo · 2022-08-12T23:30:06Z

+  name: string
+  siloRole: SiloRole | undefined
+  orgRole: OrganizationRole | undefined
+  projectRole: ProjectRole | undefined


Strictly speaking there should always be at least one of the three roles. I considered using type-fest's RequireAtLeastOne to represent that, but meh. The reason not to is there's nothing in the code that depends on it, because the cells are only looking at their own value, not the whole set.

I think it's fine. In the case where at least one role is provided that sounds like an error case we'd need to handle at runtime anyway.

just-be-dev · 2022-08-15T19:57:26Z

+  await expectRowVisible(table, {
+    ID: 'user-1',
+    Name: 'Hannah Arendt',
+    'Silo role': 'admin',
+    'Org role': '',
+  })
+  await expectRowVisible(table, {
+    ID: 'user-2',
+    Name: 'Hans Jonas',
+    'Silo role': '',
+    'Org role': 'viewer',
+  })


These really are so much better, ha

david-crespo · 2022-08-15T22:14:29Z

 import type { OrganizationRole, ProjectRole, SiloRole } from '@oxide/api'
 import { Badge } from '@oxide/ui'

-// I think there are changes in RT 8.4+ that make this less ridiculous


I was right

david-crespo · 2022-08-16T02:22:21Z

-export const getOrgRole = getMainRole(orgRoleOrder)
+export const getEffectiveOrgRole = (
+  roles: OrganizationRole[]
+): OrganizationRole | undefined => sortBy(roles, (role) => orgRoleOrder[role])[0]


the changes from here up are just cleanup and renaming. I didn't like this stuff, now I feel slightly better about it

david-crespo · 2022-08-16T02:25:15Z

-      // wouldn't be a group
-      roleName: getMainRole(roleOrder)(groupRoleAssignments.map((ra) => ra.roleName))!,
-    }))
-  }, [policy, usersDict, roleOrder])


The main change is that we are no longer doing a groupBy here — we're doing it in the calling code instead, in the project and org access pages. useUserRows is much easier to understand now: it takes a list of role assignments and adds some info to each one with a simple map.

just-be-dev · 2022-08-16T02:51:48Z

Are there any other changes pending before this PR can be merged? I know there was some discussion around design improvements, but I wasn't sure if that was in scope with this PR.

david-crespo · 2022-08-16T04:34:54Z

Nope, this is complete. I will follow up with a look at groups, but I don’t think there is currently a way to fetch a group or a list of groups from the API, so that work will probably have to wait a week or two.

just-be-dev · 2022-08-16T14:06:33Z

+  if (!cellRole) return null
+  const effectiveRole = info.row.original.effectiveRole
+  return (
+    <Badge color={effectiveRole === cellRole ? 'default' : 'neutral'}>{cellRole}</Badge>


Visually secondary might be a better fit here. It definitely can wait until a design pass though.

This might not be the right place for it, but having tooltips on the non-effective roles would be good.

Yeah, definitely. Eugene and Ben are thinking about all that.

just-be-dev · 2022-08-16T14:11:03Z

+    apiQueryClient.prefetchQuery('policyView', {}),
    apiQueryClient.prefetchQuery('organizationPolicyView', requireOrgParams(params)),


When you demo this, it'd be good to call out that pagination would be a challenge. As I'm thinking about this, it almost seems like these endpoints should more be policyList and organizationPolicyList with /policy/<user-id> being a valid lookup. If we did that then we could just use policy list and do a lookup (or future batch lookup) for the related endpoints. Or it's just PolicyView with a filter? Either way..

Again, nothing to do in this PR.

just-be-dev · 2022-08-16T14:12:53Z

+  const orgRows = useUserRows(orgPolicy?.roleAssignments, 'org')
+  const siloRows = useUserRows(siloPolicy?.roleAssignments, 'silo')
+  const rows = useMemo(() => {
+    const users = groupBy(siloRows.concat(orgRows), (u) => u.id).map(([id, ras]) => {


What is ras short for? Role assignments?

yeah. it sucks, I'll fix it and then merge

just-be-dev

Whew, tough little piece of work. Good job though, I think this'll really be helpful in visualizing some of the challenges around our IAM model.

Good to merge when you're ready 👍

david-crespo added 3 commits August 9, 2022 16:09

most basic possible policy merge

c45610b

bump api to silo policy endpoint PR branch

8273c34

merge silo, org policies for org IAM; silo, org, project for project IAM

b50c44a

Merge main into merge-policy

f0a02f8

vercel Bot deployed to Preview August 12, 2022 04:39 View deployment

david-crespo added 3 commits August 12, 2022 12:44

simplify IAM merge, don't group by user (will probably put it back)

2e28933

bring the Object.entries inside groupBy

f0da4f1

enter the matrix

ba9f2b1

vercel Bot deployed to Preview August 12, 2022 19:54 View deployment

david-crespo added 3 commits August 12, 2022 15:51

highlight effective role in green

ca73e81

make e2e tests pass

d9bcb23

add test for new table behavior to e2e

9e0095d

vercel Bot deployed to Preview August 12, 2022 21:06 View deployment

david-crespo marked this pull request as ready for review August 12, 2022 21:39

point to omicron main now that the change is merged

3c7ef8c

vercel Bot deployed to Preview August 12, 2022 21:46 View deployment

david-crespo commented Aug 12, 2022

View reviewed changes

disable change and delete instead of removing

22d37b1

david-crespo force-pushed the merge-policy branch from e6a713f to 22d37b1 Compare August 12, 2022 23:21

david-crespo commented Aug 12, 2022

View reviewed changes

vercel Bot deployed to Preview August 12, 2022 23:28 View deployment

david-crespo commented Aug 12, 2022

View reviewed changes

david-crespo requested a review from just-be-dev August 12, 2022 23:31

just-be-dev reviewed Aug 15, 2022

View reviewed changes

david-crespo added 2 commits August 15, 2022 17:06

Merge main into merge-policy

3ad32dd

use RT 8.5 to do less weird type stuff

ceabf51

vercel Bot deployed to Preview August 15, 2022 22:11 View deployment

david-crespo commented Aug 15, 2022

View reviewed changes

david-crespo added 3 commits August 15, 2022 17:26

less confusing row processing, sort by name instead of id

7dae6dd

whoops, orgs have no project role

2496fd0

slightly easier to understand roles nonsense

a3788c4

vercel Bot deployed to Preview August 15, 2022 23:15 View deployment

david-crespo commented Aug 16, 2022

View reviewed changes

inline useUsersDict. save 9 lines

cb7960f

vercel Bot deployed to Preview August 16, 2022 02:42 View deployment

just-be-dev reviewed Aug 16, 2022

View reviewed changes

just-be-dev approved these changes Aug 16, 2022

View reviewed changes

rename some variables. let's be kind to our future selves

705d637

vercel Bot deployed to Preview August 16, 2022 15:01 View deployment

david-crespo enabled auto-merge (squash) August 16, 2022 15:02

david-crespo merged commit cb77585 into main Aug 16, 2022

david-crespo deleted the merge-policy branch August 16, 2022 15:03

david-crespo mentioned this pull request Aug 23, 2022

"Inherited" permissions not included in policy response oxidecomputer/omicron#1326

Closed

		apiQueryClient.prefetchQuery('policyView', {}),
		apiQueryClient.prefetchQuery('organizationPolicyView', requireOrgParams(params)),

Uh oh!

Conversation

david-crespo commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's in here

Followup work

Uh oh!

vercel Bot commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev commented Aug 16, 2022

Uh oh!

david-crespo commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-be-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

david-crespo commented Aug 11, 2022 •

edited

Loading

vercel Bot commented Aug 11, 2022 •

edited

Loading

david-crespo Aug 12, 2022 •

edited

Loading

david-crespo commented Aug 16, 2022 •

edited

Loading

just-be-dev Aug 16, 2022 •

edited

Loading

just-be-dev Aug 16, 2022 •

edited

Loading